Fix : Remove default maxSessionDuration for AgentRuntime#410
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes maxSessionDuration optional for AgentRuntime by removing its default value and required validation in both the CRD and Go API definitions. It also updates the workload builder logic to only set a sandbox shutdown time if a TTL is specified, defaulting to DefaultSandboxTTL specifically for code interpreters, and adds a corresponding unit test assertion. Feedback highlights that maxSessionDuration is still listed in the required list at the bottom of the CRD file, which will cause Kubernetes to reject resources that omit it.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR aims to resolve Issue #303 by making AgentRuntime.spec.maxSessionDuration optional (removing the implicit 8h hard lifetime), while preserving the existing default max session duration behavior for CodeInterpreter. It updates sandbox construction so a hard ShutdownTime is only set when explicitly configured, updates the AgentRuntime API/CRD, and adjusts unit tests accordingly.
Changes:
- Remove the default hard max lifetime for AgentRuntime sessions by not setting
Sandbox.Spec.Lifecycle.ShutdownTimeunlessmaxSessionDurationis provided. - Preserve CodeInterpreter’s default hard max lifetime (8h) by explicitly applying
DefaultSandboxTTLwhenmaxSessionDurationis omitted. - Update the AgentRuntime API/CRD and unit tests to reflect the new optional semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/workloadmanager/workload_builder.go |
Stop defaulting TTL in the generic sandbox builder; set ShutdownTime only when TTL is explicitly configured; preserve CodeInterpreter TTL defaulting. |
pkg/workloadmanager/workload_builder_test.go |
Update AgentRuntime default timeout test to assert ShutdownTime is nil when maxSessionDuration is omitted. |
pkg/apis/runtime/v1alpha1/agent_type.go |
Remove kubebuilder-required/default markers and document MaxSessionDuration as optional for AgentRuntime. |
manifests/charts/base/crds/runtime.agentcube.volcano.sh_agentruntimes.yaml |
Remove the CRD default value for maxSessionDuration. |
Comments suppressed due to low confidence (1)
manifests/charts/base/crds/runtime.agentcube.volcano.sh_agentruntimes.yaml:50
- The Helm CRD still marks
spec.maxSessionDurationas required even though the default was removed. In this CRD,spec.requiredstill includes- maxSessionDuration(see same file around line 8505), so clients will still be forced to provide the field, which contradicts making it optional inAgentRuntimeSpec.
Update the CRD schema to remove maxSessionDuration from the spec.required list (ideally by regenerating the CRD from controller-gen after removing the +kubebuilder:validation:Required tag in pkg/apis/runtime/v1alpha1/agent_type.go).
maxSessionDuration:
description: |-
MaxSessionDuration describes the maximum duration for a session.
After this duration, the session will be terminated no matter active or inactive.
type: string
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
===========================================
+ Coverage 47.57% 58.56% +10.99%
===========================================
Files 30 36 +6
Lines 2819 3468 +649
===========================================
+ Hits 1341 2031 +690
+ Misses 1338 1228 -110
- Partials 140 209 +69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This matches the direction discussed in #303: AgentRuntime should not get an implicit 8h hard lifetime, while CodeInterpreter can keep the existing default. Please run |
done, Please check. |
| var shutdownTime *metav1.Time | ||
| if params.ttl != 0 { | ||
| st := metav1.NewTime(time.Now().Add(params.ttl)) | ||
| shutdownTime = &st | ||
| } |
Make maxSessionDuration optional by falling back to DefaultSandboxTTL when unset. Document the non-positive maxSessionDuration behavior, make the noExpiryDeadline sentinel unexported, and strengthen the default TTL fallback test. Signed-off-by: Safiya <147792763+safiya2610@users.noreply.github.com>
|
@acsoto ig, everything is done, Please review. |
What type of PR is this?
This PR resolves Issue #303 by making AgentRuntime.spec.maxSessionDuration optional instead of defaulting to 8 hours.
Changes
Removed the default maxSessionDuration value for AgentRuntime.
Updated the workload builder to set Sandbox.Spec.Lifecycle.ShutdownTime only when maxSessionDuration is explicitly configured.
Preserved the existing default maximum session duration behavior for CodeInterpreter.
Updated the AgentRuntime CRD to reflect the optional field.
Added and updated unit tests to cover the new behavior.
Why
Previously, every AgentRuntime was assigned a default maximum session duration of 8 hours, causing long-running agents to be terminated even when they were still active. With this change, long-running AgentRuntime instances are no longer force-terminated unless a maximum session duration is explicitly specified, while CodeInterpreter continues to retain its existing behavior.
Testing
go test ./pkg/workloadmanager/...
go test ./pkg/apis/runtime/v1alpha1/...
go build ./...
Which issue(s) this PR fixes:
Fixes #303